[INS-255] Updated datadog detector to set verificationError in case of a verification error#4661
Conversation
MuneebUllahKhan222
changed the title
MuneebUllahKhan222
changed the base branch from
datadog-detector-verification-fix
to
main
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| appIncluded = true | ||
| results = append(results, s1) | ||
| } | ||
|
|
There was a problem hiding this comment.
Unused API response causes missing user/org data
High Severity
The verifyMatch function returns the decoded *userServiceResponse as res, but the code creates a new empty var serviceResponse userServiceResponse and checks its fields instead of using res. Since serviceResponse is freshly declared and empty, len(serviceResponse.Data) and len(serviceResponse.Included) will always be 0, so setUserEmails and setOrganizationInfo are never called. The res returned from verifyMatch is completely ignored.
| var uniqueFoundUrls = make(map[string]struct{}) | ||
| for _, matches := range datadogURLPat.FindAllStringSubmatch(dataStr, -1) { | ||
| uniqueFoundUrls["https://"+matches[1]] = struct{}{} | ||
| } | ||
| endpoints := make([]string, 0, len(uniqueFoundUrls)) | ||
| for endpoint := range uniqueFoundUrls { | ||
| endpoints = append(endpoints, endpoint) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why not directly append found urls into a slice?
| for _, baseURL := range s.Endpoints(endpoints...) { | ||
| client := s.getClient() | ||
| res, isVerified, verificationErr := verifyMatch(ctx, client, resApiMatch, resAppMatch, baseURL) | ||
| s1.Verified = isVerified | ||
| s1.SetVerificationError(verificationErr, resApiMatch, resAppMatch) | ||
| if isVerified && res != nil { | ||
| s1.ResetVerificationError() // Reset verification error in case a secret is verified with an endpoint | ||
| s1.AnalysisInfo = map[string]string{"apiKey": resApiMatch, "appKey": resAppMatch, "endpoint": baseURL} | ||
| var serviceResponse userServiceResponse | ||
| if len(serviceResponse.Data) > 0 { | ||
| setUserEmails(serviceResponse.Data, &s1) |
There was a problem hiding this comment.
Can you add some empty lines in between and some comments to have better readability?
| appIncluded = true | ||
| results = append(results, s1) | ||
| } | ||
|
|
| s1.Verified = isVerified | ||
| s1.SetVerificationError(verificationErr, resApiMatch, resAppMatch) | ||
| if isVerified && res != nil { | ||
| s1.ResetVerificationError() // Reset verification error in case a secret is verified with an endpoint |
There was a problem hiding this comment.
SetVerificationError only set the error if the error is not nil than why we need to reset an error?
MuneebUllahKhan222
changed the base branch from
main
to
datadog-detector-verification-fix
MuneebUllahKhan222
changed the base branch from
datadog-detector-verification-fix
to
main
| switch res.StatusCode { | ||
| case http.StatusOK: | ||
| var serviceResponse userServiceResponse | ||
| if err := json.NewDecoder(res.Body).Decode(&serviceResponse); err != nil { | ||
| return nil, false, err | ||
| } | ||
| return &serviceResponse, true, nil | ||
| case http.StatusBadRequest: | ||
| return nil, false, fmt.Errorf("bad request") | ||
| case http.StatusUnauthorized: | ||
| return nil, false, fmt.Errorf("invalid credentials") | ||
| case http.StatusForbidden: | ||
| return nil, false, fmt.Errorf("Insufficient permissions") | ||
| case http.StatusTooManyRequests: | ||
| return nil, false, fmt.Errorf("too many requests") | ||
| default: | ||
| return nil, false, fmt.Errorf("unexpected status code: %d", res.StatusCode) | ||
| } |
There was a problem hiding this comment.
Do you think we should return an error in case we receive a 401? I mean, the credential is confirmed invalid, so there shouldn't be any error returned.
Similarly, should 403 return false with an error? Insufficient permissions error means the credentials are valid but lack permission to perform the operation.
Also, we should reduce the cases by merging similar ones. Normally, we have these cases:
valid:truewith no errorvalid:falsewith no errorvalid:falsewith error
3 participants
Description:
According to this GitHub issue, secrets detected by the DatadogToken detector that failed verification were not being reported to the notifier worker (CLI output).
This PR updates the DatadogToken detector to follow the same verification pattern used by newer detectors by Setting
verificationErroron thedetectors.Resultwith the error returned fromverifyMatchfunction.With this change, unverified Datadog token findings are correctly propagated to the notifier worker and reported in CLI output.
This PR also introduces
resetVerificationErrorfunction which reset theverificationErrorfield on the results struct required when the secret is verified with any of the endpoint found/configured so as to not report a false verification error.Checklist:
make test-community)?make lintthis requires golangci-lint)?